Added consistent hashing strategy #1087

jaronoff97 · 2022-09-12T22:27:49Z

This PR adds the consistent hashing strategy. In doing so, I included tests to verify some of the expected behaviors of this strategy method. This PR is one solution to close #1061 as two target allocator replicas will be able to run simultaneously with this strategy because they will always provide the same answer.

spec:
  targetAllocator:
    allocationStrategy: "consistent-hashing"
    replicas: 2
    enabled: true

jaronoff97 · 2022-09-12T22:28:05Z

cmd/otel-allocator/allocation/consistent_hashing.go

+		PartitionCount:    1061,
+		ReplicationFactor: 5,
+		Load:              1.1,
+		Hasher:            hasher{},


question: should this be configurable

jaronoff97 · 2022-09-12T22:28:51Z

cmd/otel-allocator/allocation/consistent_hashing.go

+	return xxhash.Sum64(data)
+}
+
+type consistentHashingAllocator struct {


A lot of this code looks very similar to the least weighted strategy. It may be worth investigating in the future if we can combine these as I tried to do in #1068

kristinapathak

Actual logic looks great! It is extremely similar to the least weighted algorithm...

cmd/otel-allocator/allocation/consistent_hashing.go

…nto consistent-hashing-v2

Aneurysm9 · 2022-09-14T15:56:42Z

apis/v1alpha1/opentelemetrycollector_types.go

+	// Replicas is the number of pod instances for the underlying TargetAllocator, this can only be set to values other
+	// than 1 if a strategy that allows for high availability is chosen.


Suggested change

// Replicas is the number of pod instances for the underlying TargetAllocator, this can only be set to values other

// than 1 if a strategy that allows for high availability is chosen.

// Replicas is the number of pod instances for the underlying TargetAllocator, this should only be set to values other

// than 1 if a strategy that allows for high availability is chosen.

Is there an enforcement mechanism, or is this only advisory?

Which are the strategies that allow HA?

AllocationStrategy is a string how does the user know how to configure it?

// AllocationStrategy determines which strategy the target allocator should use for allocation AllocationStrategy string `json:"allocationStrategy,omitempty"`

This is only advisory, the logic for enforcing may be simple now, but will be more complicated in the future when we have things like leader election or maybe even another method like distributed state. I also was thinking if someone wanted to plug in their own TA that the operator would only know the image of, we wouldn't have a way of changing the validated webhook to know to accept the custom TA strategy.

Aneurysm9 · 2022-09-14T15:58:33Z

apis/v1alpha1/opentelemetrycollector_webhook.go

 		r.Spec.Replicas = &one
 	}
+	if r.Spec.TargetAllocator.Enabled && r.Spec.TargetAllocator.Replicas == nil {
+		r.Spec.TargetAllocator.Replicas = &one


Is it safe to share the pointer here? If these values are ever modified is it always by assigning a new pointer or is it ever possible that the underlying value will be changed?

I think it's okay to modify because this is just for the webhook. The object passed to the operator wouldn't have a spec with a shared pointer (this worked in my testing)

Aneurysm9 · 2022-09-14T16:00:31Z

cmd/otel-allocator/allocation/consistent_hashing.go

+		PartitionCount:    1061,
+		ReplicationFactor: 5,
+		Load:              1.1,


Should these configuration options be exposed to the operator?

I'm not sure... I think i'm going to open a follow up issue to make these configurable as it would require some refactoring of how configuration is passed down to the allocation strategies as right now it's only the string. Is that alright?

Yeah, that's fine. I think using the string for configuration might be fine, as long as it's well specified. Something like URL query params style might work:

allocationStrategy: "consistentHashing?load=1.5&partitionCount=3000"

That would make it a straightforward addition while leaving plenty of flexibility. Don't need to solve now though, can figure it out on the follow up issue.

Aneurysm9 · 2022-09-14T19:48:30Z

cmd/otel-allocator/allocation/consistent_hashing.go

+		// Do nothing if the item is already there
+		if _, ok := c.targetItems[k]; ok {
+			continue
+		} else {


Is this necessary? addTargetToTargetItems() has provision for when the new target has an existing collector attached to it, which I think would only happen when the target existed previously as well.

I believe this is still necessary. the provision in addTargetToTargetItems() is for when we reassign a collector after a new collector is added which shifts the amount of members in the hashring. In this case, we are iterating through the newly added targets and seeing if the target has already been added. Because we're using consistent hashing, the target would get unassigned and then reassigned to the same collector which seems like unnecessary work.

pavolloffay · 2022-09-15T09:57:47Z

apis/v1alpha1/opentelemetrycollector_types.go

+	// Replicas is the number of pod instances for the underlying TargetAllocator, this can only be set to values other
+	// than 1 if a strategy that allows for high availability is chosen.


Which are the strategies that allow HA?

AllocationStrategy is a string how does the user know how to configure it?

// AllocationStrategy determines which strategy the target allocator should use for allocation AllocationStrategy string `json:"allocationStrategy,omitempty"`

cmd/otel-allocator/allocation/consistent_hashing.go

…nto consistent-hashing-v2

kristinapathak

looks great! a few grammar suggestions.

config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml

pavolloffay

approving based on the approvals from TA contributors.

…#1087) * Added consistent hashing strategy * testing fix, metrics fix * Add configurable replicas * Fix linting * Fix test and add a new one * Rename var to rerun tests * Comments and things * Grammar * docs 🤦

Added consistent hashing strategy

df48eb9

jaronoff97 commented Sep 12, 2022

View reviewed changes

kristinapathak reviewed Sep 12, 2022

View reviewed changes

cmd/otel-allocator/allocation/consistent_hashing.go Outdated Show resolved Hide resolved

cmd/otel-allocator/allocation/consistent_hashing.go Outdated Show resolved Hide resolved

kristinapathak reviewed Sep 12, 2022

View reviewed changes

cmd/otel-allocator/allocation/consistent_hashing.go Show resolved Hide resolved

jaronoff97 added 6 commits September 13, 2022 14:19

testing fix, metrics fix

64607dc

Merge branch 'main' of github.com:jaronoff97/opentelemetry-operator i…

50ceaf6

…nto consistent-hashing-v2

Add configurable replicas

1ce7876

Fix linting

15d2cab

Fix test and add a new one

d8be427

Rename var to rerun tests

ed23ab3

jaronoff97 marked this pull request as ready for review September 14, 2022 15:49

jaronoff97 requested a review from a team September 14, 2022 15:49

Aneurysm9 reviewed Sep 14, 2022

View reviewed changes

pavolloffay reviewed Sep 15, 2022

View reviewed changes

jaronoff97 added 3 commits September 15, 2022 11:27

Comments and things

abca69e

Merge branch 'main' of github.com:jaronoff97/opentelemetry-operator i…

dd94eb4

…nto consistent-hashing-v2

Merge branch 'main' of github.com:jaronoff97/opentelemetry-operator i…

0e5c854

…nto consistent-hashing-v2

jaronoff97 requested a review from Aneurysm9 September 15, 2022 17:18

kristinapathak approved these changes Sep 15, 2022

View reviewed changes

config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml Outdated Show resolved Hide resolved

config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml Outdated Show resolved Hide resolved

jaronoff97 added 2 commits September 15, 2022 13:53

Grammar

19ed494

docs 🤦

df0794e

Aneurysm9 approved these changes Sep 15, 2022

View reviewed changes

pavolloffay reviewed Sep 16, 2022

View reviewed changes

pavolloffay approved these changes Sep 16, 2022

View reviewed changes

pavolloffay merged commit 5c3e286 into open-telemetry:main Sep 16, 2022

jaronoff97 mentioned this pull request Sep 16, 2022

Allow the specification of allocation strategy configuration in collector CRD #1105

Closed

kristinapathak mentioned this pull request Sep 30, 2022

REQUEST: New membership for kristinapathak open-telemetry/community#1213

Closed

6 tasks

jaronoff97 mentioned this pull request Nov 28, 2023

[target-allocator] How does allocator itself implement HA #974

Closed

jaronoff97 mentioned this pull request Dec 22, 2023

Target Allocator is not capable of scaling out with least-weighted strategy #2477

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added consistent hashing strategy #1087

Added consistent hashing strategy #1087

jaronoff97 commented Sep 12, 2022 •

edited

Loading

jaronoff97 Sep 12, 2022

jaronoff97 Sep 12, 2022

kristinapathak left a comment

Aneurysm9 Sep 14, 2022

pavolloffay Sep 15, 2022

jaronoff97 Sep 15, 2022

Aneurysm9 Sep 14, 2022

jaronoff97 Sep 15, 2022

Aneurysm9 Sep 14, 2022

jaronoff97 Sep 15, 2022

Aneurysm9 Sep 15, 2022

Aneurysm9 Sep 14, 2022

jaronoff97 Sep 15, 2022

pavolloffay Sep 15, 2022

kristinapathak left a comment

pavolloffay left a comment

		// Replicas is the number of pod instances for the underlying TargetAllocator, this can only be set to values other
		// than 1 if a strategy that allows for high availability is chosen.

Added consistent hashing strategy #1087

Added consistent hashing strategy #1087

Conversation

jaronoff97 commented Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristinapathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristinapathak left a comment

Choose a reason for hiding this comment

pavolloffay left a comment

Choose a reason for hiding this comment

jaronoff97 commented Sep 12, 2022 •

edited

Loading